Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove a panic in normalize middleware #1762

Merged
merged 2 commits into from
Dec 1, 2020
Merged

Remove a panic in normalize middleware #1762

merged 2 commits into from
Dec 1, 2020

Conversation

maciejhirsz
Copy link
Contributor

@maciejhirsz maciejhirsz commented Oct 29, 2020 •
edited by robjtede
Loading

PR Type

Big Fix

PR Checklist

  • Tests for the changes have been added / updated.
  • A changelog entry has been made for the appropriate packages.
  • Format code with the latest stable rustfmt

Overview

Fixes a panic that occurred occasionally when using path normalization middleware. If the original path was empty due to a malformed request, parts.path_and_query would be None. Actual panic logged on a live system 🙃:

Oct 29 14:46:09 polkadot-telemetry-0 a1e8ee04ee4a[678]: thread 'actix-rt:worker:2' panicked at 'called `Option::unwrap()` on a `None` value', /usr/local/cargo/registry/src/github.com-1ecc6299db9ec823/actix-web-3.1.0/src/middleware/normalize.rs:140:22

@maciejhirsz maciejhirsz mentioned this pull request Oct 29, 2020
Merged
@robjtede robjtede added A-web project: actix-web B-semver-patch labels Oct 29, 2020
JohnTitor
JohnTitor approved these changes Nov 2, 2020
View reviewed changes
Copy link
Member

@JohnTitor JohnTitor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me 👍

robjtede
robjtede reviewed Nov 2, 2020
View reviewed changes
CHANGES.md Outdated
Comment on lines 20 to 22
### Fixed
* Removed an occasional `unwrap` on `None` panic in `NormalizePathNormalization`.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move to unreleased section

@robjtede
Copy link
Member

robjtede commented Nov 2, 2020

is it possible to contrive a test for this? whats the exact condition?

@maciejhirsz
Copy link
Contributor Author

@robjtede unfortunately my server logs couldn't give me a definitive request URL that triggered it, so I weren't able to reproduce it manually, but it was happening frequently enough to warrant me running a patched version of actix-web. From what I gathered by reading code, this fails when the path is missing either the scheme (no http: etc) or is missing a path (/ is fine).

@JohnTitor
Copy link
Member

I prefer just to merge this rather than to wait for coming up with a suitable test as this is a general improvement.

@robjtede Thoughts?

@codecov
Copy link

codecov bot commented Nov 30, 2020

Codecov Report

Merging #1762 (09ae9c9) into master (32d59ca) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1762   +/-   ##
=======================================
  Coverage   53.80%   53.80%           
=======================================
  Files         129      129           
  Lines       12266    12266           
=======================================
  Hits         6600     6600           
  Misses       5666     5666
Impacted Files Coverage Δ
src/middleware/normalize.rs 99.30% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32d59ca...09ae9c9. Read the comment docs.

@robjtede
Copy link
Member

robjtede commented Dec 1, 2020

Yep it's a sensible change regardless.

@JohnTitor JohnTitor merged commit 7981e00 into actix:master Dec 1, 2020
@JohnTitor
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robjtede robjtede robjtede left review comments

@JohnTitor JohnTitor JohnTitor approved these changes

Assignees
No one assigned
Labels
A-web project: actix-web B-semver-patch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@maciejhirsz @robjtede @JohnTitor